fix(ollama): always respond to model refresh, surface errors, and use form-edited base URL#878
fix(ollama): always respond to model refresh, surface errors, and use form-edited base URL#878navedmerchant wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe Ollama model refresh flow now uses unsaved credentials, always returns a model response, surfaces fetch errors, and provides dedicated refresh controls with loading, success, and error feedback. ChangesOllama model refresh
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Ollama
participant ApiOptions
participant webviewMessageHandler
participant OllamaProvider
User->>Ollama: Click refresh
Ollama->>ApiOptions: Read current baseUrl and apiKey
ApiOptions->>webviewMessageHandler: requestOllamaModels with values
webviewMessageHandler->>OllamaProvider: flushModels with request credentials
OllamaProvider-->>webviewMessageHandler: cache refresh result
webviewMessageHandler->>OllamaProvider: getModels with request credentials
OllamaProvider-->>webviewMessageHandler: Models or rejection
webviewMessageHandler-->>Ollama: ollamaModels with optional error
Ollama-->>User: Render success or error status
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx (1)
232-355: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for the "preserve models on refresh error" scenario.
Given the model-list-wipe issue flagged in
Ollama.tsx(settingollamaModelsto{}on any error response), consider adding a test that: loads models successfully first, then triggers a refresh that fails, and asserts the previously loaded models remain inollamaModels/the picker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx` around lines 232 - 355, Add coverage in the “Ollama Component - refresh models” tests for preserving previously loaded models after a failed refresh: complete one successful refresh, trigger another refresh, dispatch an error response, and assert the existing model remains visible in the picker or otherwise unchanged. Use the existing dispatchMessage helper and refresh-button flow, and verify the error state is shown without clearing the prior model.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx`:
- Around line 74-80: Update the `vi.mock("`@src/components/ui`", ...)` setup in
`Ollama.spec.tsx` to preserve the real barrel exports via `vi.importActual()`
and override only `Button`; ensure `ModelPicker` and `ThinkingBudget` continue
receiving their actual UI dependencies while the test-specific button behavior
remains.
---
Nitpick comments:
In `@webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx`:
- Around line 232-355: Add coverage in the “Ollama Component - refresh models”
tests for preserving previously loaded models after a failed refresh: complete
one successful refresh, trigger another refresh, dispatch an error response, and
assert the existing model remains visible in the picker or otherwise unchanged.
Use the existing dispatchMessage helper and refresh-button flow, and verify the
error state is shown without clearing the prior model.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c7ef33f-2b62-4f1b-a1ac-fee40638bb54
📒 Files selected for processing (5)
src/core/webview/__tests__/webviewMessageHandler.spec.tssrc/core/webview/webviewMessageHandler.tswebview-ui/src/components/settings/ApiOptions.tsxwebview-ui/src/components/settings/providers/Ollama.tsxwebview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx
github-actions
Bot
added
the
awaiting-review
|
@roomote Please review this PR |
|
I saw the mention, but I could not start work on this PR with the current Roomote GitHub setup. |
| if (Object.keys(newModels).length > 0) { | ||
| setRefreshStatus("success") | ||
| } else { | ||
| setRefreshStatus("error") |
There was a problem hiding this comment.
When Ollama is running but no models are installed, the backend posts { ollamaModels: {} } with no error field — so message.error is undefined here and the component shows the generic error string. Is a healthy-but-empty Ollama server really the same as a fetch failure?
| if (Object.keys(newModels).length > 0) { | |
| setRefreshStatus("success") | |
| } else { | |
| setRefreshStatus("error") | |
| } else if (message.error) { | |
| setRefreshStatus("error") | |
| setRefreshError(message.error) | |
| } else { | |
| // Ollama reachable but no models installed yet | |
| setRefreshStatus("success") |
| return () => { | ||
| window.removeEventListener("message", handleMessage) | ||
| } | ||
| }, [refreshStatus]) |
There was a problem hiding this comment.
The [refreshStatus] dep tears down and re-adds the listener on the idle→loading transition — which fires synchronously right before vscode.postMessage(...) at line 66. If Ollama responds very quickly, could the response arrive in the brief gap and be silently dropped, leaving the UI stuck in loading?
A useRef for refreshStatus with a stable [] dep would avoid the re-subscription.
| type: "requestOllamaModels", | ||
| values: { | ||
| baseUrl: apiConfiguration?.ollamaBaseUrl, | ||
| apiKey: apiConfiguration?.ollamaApiKey, |
There was a problem hiding this comment.
ollamaApiKey is now forwarded in values, but it’s not in the useDebounce dep array (further down around the dep list). Should it be, so a key-only change also triggers a re-fetch?
| // the loading state. Include the error message so the webview | ||
| // can display it directly in the settings panel. | ||
| const errorMsg = error instanceof Error ? error.message : String(error) | ||
| provider.log(`[requestOllamaModels] Failed to fetch models: ${errorMsg}`) |
There was a problem hiding this comment.
When the user has typed a new URL but not saved, this log won’t show which host was actually attempted. Worth including baseUrl?
| provider.log(`[requestOllamaModels] Failed to fetch models: ${errorMsg}`) | |
| provider.log(`[requestOllamaModels] Failed to fetch models from ${baseUrl}: ${errorMsg}`) |
| // Always post a response so the webview refresh status can | ||
| // transition out of "loading" — even when no models are found. | ||
| provider.postMessageToWebview({ type: "ollamaModels", ollamaModels: ollamaModels }) | ||
| } catch (error) { |
There was a problem hiding this comment.
Both flushModels and getModels can throw into this catch, but the log says "Failed to fetch models" in both cases. A cache-layer failure and a network failure would look identical in the output channel — worth distinguishing?
| apiKey: "secret-key", | ||
| }) | ||
|
|
||
| expect(mockClineProvider.postMessageToWebview).toHaveBeenCalledWith({ |
There was a problem hiding this comment.
mockGetModels is asserted with the right options, but mockFlushModels isn’t checked here. If flushModels used the stale saved-state URL while getModels used the form-edited one, this test would still pass. Worth adding:
expect(mockFlushModels).toHaveBeenCalledWith(
{ provider: "ollama", baseUrl: "https://ollama.example.com", apiKey: "secret-key" },
true
)
3 participants
Related GitHub Issue
Closes: #877
Description
This PR fixes the Ollama model refresh flow so the settings panel always gives the user feedback and uses the URL they're actually looking at. The key implementation details:
Extension handler (
webviewMessageHandler.ts)requestOllamaModelscase now prefersbaseUrl/apiKeyfrom the incoming messagevalues(which reflect the user's unsaved form edits) over the saved extension state, falling back to state only when values are absent. This lets a user validate a URL before saving it.ollamaModelsresponse back to the webview — even when no models are found — so the UI can transition out of the loading state instead of hanging forever.provider.log(...)and an emptyollamaModelsresponse is posted with the error message included, so the webview can display it directly. The previousconsole.debugsilent swallow is removed.Webview (
ApiOptions.tsx)requestOllamaModelsmessage dispatched on provider switch now includesvalues: { baseUrl, apiKey }from the currentapiConfiguration, matching the new handler contract.Ollama settings component (
Ollama.tsx)useEventhook fromreact-usewith a nativewindow.addEventListener("message", ...)inside auseEffect(with proper cleanup), removing thereact-usedependency from this component.refreshStatusstate machine (idle→loading→success|error) and arefreshErrorstate to hold the backend error message.Buttoncomponent) that shows a spinner while loading, disables itself during refresh, and displays success / error status text below the button. The error text prefers the backend-provided message and falls back to a localized generic error string.loadingstate, so unsolicitedollamaModelsmessages (e.g. from mount refresh) don't clobber the status display.Test Procedure
Unit tests — extension handler (
src/core/webview/__tests__/webviewMessageHandler.spec.ts)cd src && npx vitest run core/webview/__tests__/webviewMessageHandler.spec.tsbaseUrl/apiKeyfrom messagevaluestake precedence over saved state.Unit tests — Ollama component (
webview-ui/src/components/settings/providers/__tests__/Ollama.spec.tsx)cd webview-ui && npx vitest run src/components/settings/providers/__tests__/Ollama.spec.tsxdescribe("Ollama Component - refresh models")block verifies: idle button render, correctrequestOllamaModelspayload withvalueson click, loading/disabled state, success state on models arrival, error state on empty models, backend error message display, and no-op for unsolicited messages when not loading.Manual testing
Pre-Submission Checklist
Screenshots / Videos
N/A — the refresh button and status text use existing shared UI primitives (
Button, codicon icons, VS Code CSS variables) with no new visual design.Documentation Updates
settings:providers.refreshModels.*).Additional Notes
The
react-useuseEventimport was removed fromOllama.tsxin favor of a nativeuseEffect-based listener. This is a minor cleanup that reduces the component's dependency surface and makes the listener lifecycle explicit. The corresponding test mock forreact-usewas also removed.Get in Touch
navedmerchant (GitHub) / navedmerchant on Discord
Summary by CodeRabbit
ollamaModelspayload to the UI, including empty results and explicit error details.